PR 5: refactor(effort): component extraction + report context centralization#178
Open
rlorenzo wants to merge 12 commits into
Open
PR 5: refactor(effort): component extraction + report context centralization#178rlorenzo wants to merge 12 commits into
rlorenzo wants to merge 12 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughPR extracts reusable Vue component primitives (\ ChangesVue Component Extraction & Refactoring
Backend Service Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5 tasks
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
eee8d10 to
8f5ffd2
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ebe32ec to
75abeb2
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
8f5ffd2 to
9cad656
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
4 times, most recently
from
d26c5d8 to
8cbfb69
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
9cad656 to
d14e7b9
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
2 times, most recently
from
0d62a4e to
a0c55c4
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
d14e7b9 to
94b732b
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
a0c55c4 to
d8a3735
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
94b732b to
61ee064
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
d8a3735 to
ddd64e7
Compare
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
61ee064 to
f53d1a9
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ddd64e7 to
66fe537
Compare
Hoist breadcrumb, loading spinner, and not-found banner from EmergencyContactForm.vue and EmergencyContactView.vue into a new EmergencyContactPageShell.vue. Each page now owns only its h1 and body content. Pure template refactor; no runtime change.
AddCourseEffortDialog and EffortRecordEditDialog shared a Cancel + primary-action q-card-actions block with an identical #loading slot. Hoist into DialogSubmitActions.vue parameterized by submitLabel and isSaving. Other Effort dialogs still inline the pattern; migrate as they are touched.
PercentAssignmentAddDialog and PercentAssignmentEditDialog shared a 165-line q-select/q-input/q-checkbox form body. Hoist into PercentAssignmentFormFields.vue, v-modeled on the form state the usePercentageForm composable already centralized. Each dialog now owns only its title, save logic, banner variants, and footer. Also migrate both footers to DialogSubmitActions. PercentageFormState and TypeOption are re-exported from the composable so the new component can type its props.
Consolidates the h1 + ReportFilterForm + loading spinner + ReportLayout header/ExportToolbar + empty-state boilerplate that was duplicated across all 9 standard Effort report pages into a single wrapper, so individual pages only define their table content.
Share the Role / Effort Value / Notes / warning + error banner block between EffortRecordAddDialog and EffortRecordEditDialog, and route the Add dialog's footer through the existing DialogSubmitActions component.
Share the Instructors breadcrumb, loading spinner, and error banner between InstructorDetail and InstructorEdit via a slot-based shell.
Share the dialog scaffold (close affordance, title/subtitle, loading spinner, progress bar, and error state) between HarvestDialog, PercentRolloverDialog, and ClinicalImportDialog. Per-dialog preview bodies and action buttons stay in the consumers.
Share the desktop table + mobile list rendering across unopened / open / closed term sections instead of inlining three near-identical q-table blocks.
Move ITermService injection into BaseReportService and add shared helpers (LoadSingleTermContextAsync, LoadYearlyReportContextAsync, ExtractDistinctEffortTypes) so DeptSummary, SchoolSummary, and TeachingActivity services no longer hand-roll the same row + clinical-faculty + term-name plumbing.
…tService Replace four near-identical N5..N1 weighted-average + divide-by-zero guards in the summary and detail builders with a single helper.
The InstructorPageShell extraction (refactor 4601108) replaced the StatusBanner import in InstructorEdit.vue, but the template still references StatusBanner three times for save/error feedback. Vue logged "Failed to resolve component: StatusBanner" warnings and rendered those banners blank.
Carry-over from the analyzer-driven cleanup batch (PR 3). The materialization fix targets the helper extracted in d12711e — that helper doesn't exist in PR 3's base, so the fix moves here.
rlorenzo
force-pushed
the
refactor/effort-component-extraction
branch
from
f53d1a9 to
9518c20
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@VueApp/src/Effort/components/AsyncOperationDialog.vue`:
- Line 45: Remove the inline style attributes on the QCard and the element at
the top-right: replace the :style binding on the q-card (the element using
`:style="`width: 100%; max-width: ${maxWidth}; position: relative`"`) with a
class (e.g., `async-op-card`) and move the top-right element's inline z-index
style into a class (e.g., `absolute-top-right`); then add those CSS rules in the
component's <style scoped> block (set width/ max-width/position rules for
`.async-op-card` and z-index/positioning for `.absolute-top-right`) and update
the template to use the new class names or Quasar utility classes instead of
inline styles.
In `@VueApp/src/Effort/components/ClinicalImportDialog.vue`:
- Around line 151-156: The Confirm Import QBtn in ClinicalImportDialog.vue (the
<q-btn> with label="Confirm Import", color="info", :disable="totalChanges === 0
|| isCommitting", `@click`="confirmImport") needs the text-color="dark" attribute
added to satisfy the UI guidelines and ensure sufficient contrast on the info
background; update that <q-btn> to include text-color="dark" (and mirror the
same change for any other info/warning QBtn instances in this component if
present).
In `@VueApp/src/Effort/components/EffortReportPage.vue`:
- Around line 20-21: Replace the callback-prop pattern with Vue events: remove
the onPdfExport and onExcelExport props from ExportToolbar usage and instead
have ExportToolbar emit "export-pdf" and "export-excel"; update
EffortReportPage.vue to listen for those emits (e.g., <ExportToolbar
`@export-pdf`="handlePdfExport" `@export-excel`="handleExcelExport">) and rework any
prop passthrough so EffortReportPage emits its own events
(this.$emit('export-pdf') / this.$emit('export-excel') or calls local handlers)
rather than passing functions down; ensure you update the component's emits
option and any parent pages to use `@export-pdf` and `@export-excel` instead of
:on-pdf-export/:on-excel-export.
In `@VueApp/src/Effort/components/HarvestDialog.vue`:
- Around line 198-205: Replace the raw q-badge usage inside each
body-cell-status template with the project's StatusBadge component so accessible
text-color logic is preserved: for each template (e.g., the one using
getStatusColor(slotProps.value)) render StatusBadge and pass the status value
and the color computed by getStatusColor(slotProps.value) instead of q-badge,
and restore/ensure the StatusBadge import at the top of the file; apply the same
replacement to all eight body-cell-status sites referenced so they consistently
use StatusBadge rather than q-badge.
In `@VueApp/src/Effort/components/PercentAssignmentFormFields.vue`:
- Around line 67-80: The q-select for Unit (v-model="form.unitId") is marked
required via label "Unit *" and the rule requiredRule('Unit') but still includes
the clearable prop; remove the clearable attribute from the q-select element in
PercentAssignmentFormFields.vue so users cannot clear the required Unit field
and validation remains consistent.
In `@VueApp/src/Effort/components/TermTable.vue`:
- Line 18: The title is rendered twice in TermTable.vue (the <h2 class="text-h6
q-mb-sm q-mt-none">{{ title }}</h2> and the separate title inside the lt-sm
section), causing duplicate display on mobile; fix by either adding the
responsive class gt-xs to the existing <h2> so it hides on extra-small screens,
or remove the duplicate subtitle inside the lt-sm block so the single <h2>
remains; update the element that currently prints {{ title }} inside the lt-sm
section (or add gt-xs to the <h2>) accordingly to ensure only one title is shown
on mobile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01234b4d-9fac-4c89-90f3-55c6ec94e220
📒 Files selected for processing (43)
VueApp/src/Effort/components/AddCourseEffortDialog.vueVueApp/src/Effort/components/AsyncOperationDialog.vueVueApp/src/Effort/components/ClinicalImportDialog.vueVueApp/src/Effort/components/DialogSubmitActions.vueVueApp/src/Effort/components/EffortRecordAddDialog.vueVueApp/src/Effort/components/EffortRecordEditDialog.vueVueApp/src/Effort/components/EffortRecordSharedFields.vueVueApp/src/Effort/components/EffortReportPage.vueVueApp/src/Effort/components/HarvestDialog.vueVueApp/src/Effort/components/InstructorPageShell.vueVueApp/src/Effort/components/PercentAssignmentAddDialog.vueVueApp/src/Effort/components/PercentAssignmentEditDialog.vueVueApp/src/Effort/components/PercentAssignmentFormFields.vueVueApp/src/Effort/components/PercentRolloverDialog.vueVueApp/src/Effort/components/TermTable.vueVueApp/src/Effort/composables/use-percentage-form.tsVueApp/src/Effort/pages/DeptSummary.vueVueApp/src/Effort/pages/EvalDetail.vueVueApp/src/Effort/pages/EvalSummary.vueVueApp/src/Effort/pages/InstructorDetail.vueVueApp/src/Effort/pages/InstructorEdit.vueVueApp/src/Effort/pages/MeritAverage.vueVueApp/src/Effort/pages/MeritDetail.vueVueApp/src/Effort/pages/MeritSummary.vueVueApp/src/Effort/pages/SchoolSummary.vueVueApp/src/Effort/pages/TeachingActivityGrouped.vueVueApp/src/Effort/pages/TeachingActivityIndividual.vueVueApp/src/Effort/pages/TermSelection.vueVueApp/src/Students/EmergencyContact/components/EmergencyContactPageShell.vueVueApp/src/Students/EmergencyContact/pages/EmergencyContactForm.vueVueApp/src/Students/EmergencyContact/pages/EmergencyContactView.vuetest/Effort/BaseReportServiceTests.cstest/Effort/ExcelGenerationTests.csweb/Areas/Effort/Services/BaseReportService.csweb/Areas/Effort/Services/ClinicalEffortService.csweb/Areas/Effort/Services/ClinicalScheduleService.csweb/Areas/Effort/Services/DeptSummaryService.csweb/Areas/Effort/Services/EvaluationReportService.csweb/Areas/Effort/Services/MeritMultiYearService.csweb/Areas/Effort/Services/MeritReportService.csweb/Areas/Effort/Services/MeritSummaryService.csweb/Areas/Effort/Services/SchoolSummaryService.csweb/Areas/Effort/Services/TeachingActivityService.cs
| aria-labelledby="async-operation-dialog-title" | ||
| @keydown.escape="$emit('close')" | ||
| > | ||
| <q-card :style="`width: 100%; max-width: ${maxWidth}; position: relative`"> |
Contributor
There was a problem hiding this comment.
Remove inline styles.
Lines 45 and 52 use inline style attributes. Prefer Quasar utility classes or define styles in a <style scoped> block. As per coding guidelines, no inline styles are allowed.
♻️ Proposed fix
For line 45, use :style binding or move to scoped styles:
- <q-card :style="`width: 100%; max-width: ${maxWidth}; position: relative`">
+ <q-card
+ class="full-width"
+ :style="{ maxWidth, position: 'relative' }"
+ >For line 52, move z-index to scoped styles:
- style="z-index: 1"Add to <style scoped> block at the end of the file:
<style scoped>
.absolute-top-right {
z-index: 1;
}
</style>Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@VueApp/src/Effort/components/AsyncOperationDialog.vue` at line 45, Remove the
inline style attributes on the QCard and the element at the top-right: replace
the :style binding on the q-card (the element using `:style="`width: 100%;
max-width: ${maxWidth}; position: relative`"`) with a class (e.g.,
`async-op-card`) and move the top-right element's inline z-index style into a
class (e.g., `absolute-top-right`); then add those CSS rules in the component's
<style scoped> block (set width/ max-width/position rules for `.async-op-card`
and z-index/positioning for `.absolute-top-right`) and update the template to
use the new class names or Quasar utility classes instead of inline styles.
Comment on lines
151
to
156
| <q-btn | ||
| label="Retry" | ||
| color="primary" | ||
| class="q-mt-md" | ||
| @click="loadPreview" | ||
| label="Confirm Import" | ||
| color="info" | ||
| :disable="totalChanges === 0 || isCommitting" | ||
| @click="confirmImport" | ||
| /> |
Contributor
There was a problem hiding this comment.
Missing text-color="dark" on info button.
The Confirm Import button uses color="info" without the required dark text color, hurting contrast on the light info background.
♻️ Proposed fix
<q-btn
label="Confirm Import"
color="info"
+ text-color="dark"
:disable="totalChanges === 0 || isCommitting"
`@click`="confirmImport"
/>As per coding guidelines: "Use button color scheme: primary (Aggie Blue), positive (create), negative (delete), info text-color="dark" (tertiary), warning text-color="dark" (caution), secondary."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <q-btn | |
| label="Retry" | |
| color="primary" | |
| class="q-mt-md" | |
| @click="loadPreview" | |
| label="Confirm Import" | |
| color="info" | |
| :disable="totalChanges === 0 || isCommitting" | |
| @click="confirmImport" | |
| /> | |
| <q-btn | |
| label="Confirm Import" | |
| color="info" | |
| text-color="dark" | |
| :disable="totalChanges === 0 || isCommitting" | |
| `@click`="confirmImport" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@VueApp/src/Effort/components/ClinicalImportDialog.vue` around lines 151 -
156, The Confirm Import QBtn in ClinicalImportDialog.vue (the <q-btn> with
label="Confirm Import", color="info", :disable="totalChanges === 0 ||
isCommitting", `@click`="confirmImport") needs the text-color="dark" attribute
added to satisfy the UI guidelines and ensure sufficient contrast on the info
background; update that <q-btn> to include text-color="dark" (and mirror the
same change for any other info/warning QBtn instances in this component if
present).
Comment on lines
+20
to
+21
| onPdfExport: () => void | Promise<void> | ||
| onExcelExport: () => Promise<void> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Prefer events over callback props.
In Vue 3, the idiomatic pattern is to emit events rather than accept callback functions as props. Consider having ExportToolbar emit @export-pdf and @export-excel events, then emit corresponding events from this component instead of passing callbacks through.
♻️ Refactor to use events
Props:
- onPdfExport: () => void | Promise<void>
- onExcelExport: () => Promise<void>Emits:
defineEmits<{
(e: "generate", params: ReportFilterParams): void
+ (e: "export-pdf"): void
+ (e: "export-excel"): void
}>()Template:
<ExportToolbar
- :pdf-export="onPdfExport"
- :excel-export="onExcelExport"
+ `@export-pdf`="$emit('export-pdf')"
+ `@export-excel`="$emit('export-excel')"
/>Parent pages would then use @export-pdf and @export-excel instead of :on-pdf-export and :on-excel-export.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@VueApp/src/Effort/components/EffortReportPage.vue` around lines 20 - 21,
Replace the callback-prop pattern with Vue events: remove the onPdfExport and
onExcelExport props from ExportToolbar usage and instead have ExportToolbar emit
"export-pdf" and "export-excel"; update EffortReportPage.vue to listen for those
emits (e.g., <ExportToolbar `@export-pdf`="handlePdfExport"
`@export-excel`="handleExcelExport">) and rework any prop passthrough so
EffortReportPage emits its own events (this.$emit('export-pdf') /
this.$emit('export-excel') or calls local handlers) rather than passing
functions down; ensure you update the component's emits option and any parent
pages to use `@export-pdf` and `@export-excel` instead of
:on-pdf-export/:on-excel-export.
Comment on lines
+198
to
+205
| <template #body-cell-status="slotProps"> | ||
| <q-td :props="slotProps"> | ||
| <q-badge | ||
| :color="getStatusColor(slotProps.value)" | ||
| :label="slotProps.value" | ||
| /> | ||
| </q-td> | ||
| </template> |
Contributor
There was a problem hiding this comment.
Regression: raw q-badge replaces StatusBadge in 8 status cells.
Switching back to raw q-badge with getStatusColor(...) drops the accessible text-color handling that StatusBadge provides. Colors like positive/info/grey-6 will render with default light text and fail contrast on light backgrounds.
This also runs counter to the PR's deduplication goal — eight identical body-cell-status templates can collapse into a single StatusBadge call site.
♻️ Proposed fix (apply at each of the 8 sites)
<template `#body-cell-status`="slotProps">
<q-td :props="slotProps">
- <q-badge
- :color="getStatusColor(slotProps.value)"
- :label="slotProps.value"
- />
+ <StatusBadge
+ :color="getStatusColor(slotProps.value)"
+ :label="slotProps.value"
+ />
</q-td>
</template>And restore the import:
import StatusBanner from "@/components/StatusBanner.vue"
+import StatusBadge from "@/components/StatusBadge.vue"
import AsyncOperationDialog from "./AsyncOperationDialog.vue"As per coding guidelines: "Use the StatusBadge component instead of raw q-badge with manual getAccessibleTextColor."
Also applies to: 237-244, 275-282, 320-327, 359-366, 397-404, 442-449, 481-488
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@VueApp/src/Effort/components/HarvestDialog.vue` around lines 198 - 205,
Replace the raw q-badge usage inside each body-cell-status template with the
project's StatusBadge component so accessible text-color logic is preserved: for
each template (e.g., the one using getStatusColor(slotProps.value)) render
StatusBadge and pass the status value and the color computed by
getStatusColor(slotProps.value) instead of q-badge, and restore/ensure the
StatusBadge import at the top of the file; apply the same replacement to all
eight body-cell-status sites referenced so they consistently use StatusBadge
rather than q-badge.
Comment on lines
+67
to
+80
| <!-- Unit Selection --> | ||
| <q-select | ||
| v-model="form.unitId" | ||
| :options="unitOptions" | ||
| label="Unit *" | ||
| dense | ||
| options-dense | ||
| outlined | ||
| emit-value | ||
| map-options | ||
| clearable | ||
| :rules="[requiredRule('Unit')]" | ||
| lazy-rules="ondemand" | ||
| /> |
Contributor
There was a problem hiding this comment.
Remove clearable from required Unit field.
The Unit field is marked as required (line 71: label="Unit *" and line 78: requiredRule('Unit')), but line 77 includes clearable. This allows users to clear a required field, causing validation failure. Remove clearable for consistency.
🔧 Proposed fix
<q-select
v-model="form.unitId"
:options="unitOptions"
label="Unit *"
dense
options-dense
outlined
emit-value
map-options
- clearable
:rules="[requiredRule('Unit')]"
lazy-rules="ondemand"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <!-- Unit Selection --> | |
| <q-select | |
| v-model="form.unitId" | |
| :options="unitOptions" | |
| label="Unit *" | |
| dense | |
| options-dense | |
| outlined | |
| emit-value | |
| map-options | |
| clearable | |
| :rules="[requiredRule('Unit')]" | |
| lazy-rules="ondemand" | |
| /> | |
| <!-- Unit Selection --> | |
| <q-select | |
| v-model="form.unitId" | |
| :options="unitOptions" | |
| label="Unit *" | |
| dense | |
| options-dense | |
| outlined | |
| emit-value | |
| map-options | |
| :rules="[requiredRule('Unit')]" | |
| lazy-rules="ondemand" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@VueApp/src/Effort/components/PercentAssignmentFormFields.vue` around lines 67
- 80, The q-select for Unit (v-model="form.unitId") is marked required via label
"Unit *" and the rule requiredRule('Unit') but still includes the clearable
prop; remove the clearable attribute from the q-select element in
PercentAssignmentFormFields.vue so users cannot clear the required Unit field
and validation remains consistent.
|
|
||
| <template> | ||
| <div class="q-mb-lg"> | ||
| <h2 class="text-h6 q-mb-sm q-mt-none">{{ title }}</h2> |
Contributor
There was a problem hiding this comment.
Title renders twice on mobile.
The <h2> at line 18 is visible on all screen sizes, but line 63 displays the title again inside the lt-sm section. On mobile, users see both.
Add gt-xs class to the <h2> or remove the duplicate subtitle at line 63.
🎨 Proposed fix: make title responsive
Option 1 (recommended): Keep h2 always visible, remove mobile duplicate
- <div class="lt-sm">
- <div class="text-subtitle2 q-mb-xs">{{ title }}</div>
- <q-list
+ <div class="lt-sm">
+ <q-listOption 2: Show h2 on desktop only, keep mobile subtitle
- <h2 class="text-h6 q-mb-sm q-mt-none">{{ title }}</h2>
+ <h2 class="text-h6 q-mb-sm q-mt-none gt-xs">{{ title }}</h2>Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@VueApp/src/Effort/components/TermTable.vue` at line 18, The title is rendered
twice in TermTable.vue (the <h2 class="text-h6 q-mb-sm q-mt-none">{{ title
}}</h2> and the separate title inside the lt-sm section), causing duplicate
display on mobile; fix by either adding the responsive class gt-xs to the
existing <h2> so it hides on extra-small screens, or remove the duplicate
subtitle inside the lt-sm block so the single <h2> remains; update the element
that currently prints {{ title }} inside the lt-sm section (or add gt-xs to the
<h2>) accordingly to ensure only one title is shown on mobile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part 5 of 6. Stacks on top of PR #177.
Effort-area refactors driven by jscpd (duplication detection): extract repeated UI shells, dialog scaffolding, and form-field clusters into reusable components. Plus a small report-service consolidation in C#. Also folds in the
EmergencyContactshared page shell because it pairs naturally with the Effort UI patterns.What's in this PR
Vue/UI extractions
EmergencyContactPageShell— shared page shell for emergency contact formsDialogSubmitActions— submit/cancel button cluster used across multiple dialogsPercentAssignmentFormFields— shared form-field clusterEffortReportPage— page-layout shell for report screensEffortRecordSharedFields— duplicate field cluster across record dialogsInstructorPageShell— breadcrumbs + loading/error states for instructor pagesAsyncOperationDialog— preview-dialog shellTermTable— term-selection rows componentC# / report services
BaseReportServicecentralizes report context loading; per-report services now consume it instead of each loading the term/context independentlyCalculateWeightedAverageextracted inEvaluationReportServiceto replace four near-identical N5..N1 weighted-average + divide-by-zero guardsBundled fixes
fix(effort): restore StatusBanner import in InstructorEdit— theInstructorPageShellextraction dropped aStatusBannerimport the template still referenced. Pairs with the extraction commit it patches.chore(quality): materialize CalculateWeightedAverage rows once— carry-over from PR 3'smaterialize IEnumerablebatch. The helper that needed materialization is introduced in this PR, so the analyzer fix is bundled here rather than in PR 3.Commits
refactor(emergency-contact): extract shared page shellrefactor(effort): extract DialogSubmitActions componentrefactor(effort): extract PercentAssignmentFormFieldsrefactor(effort): extract EffortReportPage layout shellrefactor(effort): extract EffortRecordSharedFields from dialogsrefactor(effort): extract InstructorPageShell for breadcrumbs and statesrefactor(effort): extract AsyncOperationDialog shell for preview dialogsrefactor(effort): extract TermTable for term selection rowsrefactor(effort): centralize report context loading in BaseReportServicerefactor(effort): extract CalculateWeightedAverage in EvaluationReportServicefix(effort): restore StatusBanner import in InstructorEditchore(quality): materialize CalculateWeightedAverage rows oncePR stack
Test plan
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements